Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require viewUserList to get user by ID #2573

Closed

Conversation

askvortsov1
Copy link
Sponsor Member

@askvortsov1 askvortsov1 commented Jan 26, 2021

Fixes https://github.com/flarum/core/issues/2560

This makes it more difficult to scape the API for users.

Is there another place where this check would be more appropriate? I'm unsure about putting it in the repository since that method is used by other classes.

@askvortsov1 askvortsov1 added this to the 0.1 milestone Mar 1, 2021
@askvortsov1 askvortsov1 force-pushed the as/require-view-user-list-to-get-user-by-id branch from 68b9e80 to a4a8541 Compare March 3, 2021 14:20
@askvortsov1 askvortsov1 self-assigned this Mar 17, 2021
@askvortsov1 askvortsov1 force-pushed the as/require-view-user-list-to-get-user-by-id branch from a4a8541 to ec4fafb Compare April 4, 2021 20:18
Copy link
Member

@clarkwinkelmann clarkwinkelmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this cause issues with extensions that use GET /api/users/id to refresh a user profile? That's a hypothetical, but just like clicking on homepage refreshes the current user, an extension might generate a request to it when clicking on the same profile you are on to refresh. Would the extension have access to the required information to make the request using the correct slugging?

Actually speaking of current user, doesn't this break the exact feature I describe? Doesn't clicking on homepage or notifications refreshes the user by making a request to their own profile by ID? We would have to allow making a request by ID to the current actor at the very least.

Other thing, can't you obtain the same information by hitting PATCH /api/users/id without any attributes ?

@askvortsov1 askvortsov1 removed this from the 0.1 milestone Apr 9, 2021
@askvortsov1
Copy link
Sponsor Member Author

These are good points. Assuming the correct serializer is always used, do we even want to prevent ID-based enumeration in core?

@clarkwinkelmann
Copy link
Member

I feel like the problem of enumeration would be best solved by a community extension that would replace increment IDs with a different ID system. Preventing enumeration while still using increments seems complicated to pull off correctly. And switching to UUIDs or other ID systems is probably a bit too radical to include in core (unless we decide to switch all IDs away from increments).

@SychO9 SychO9 deleted the as/require-view-user-list-to-get-user-by-id branch January 5, 2023 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restrict ShowUserController by ID to users who can `viewUserList
2 participants